Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix null-ref in AliasSymbol.Equals #59649

Merged
merged 2 commits into from
Feb 19, 2022
Merged

Fix null-ref in AliasSymbol.Equals #59649

merged 2 commits into from
Feb 19, 2022

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Feb 18, 2022

Fixes #58999
FYI @AlekseyTs

@jcouv jcouv requested a review from a team as a code owner February 18, 2022 18:26
@jcouv jcouv self-assigned this Feb 18, 2022
@jcouv jcouv marked this pull request as draft February 18, 2022 18:26
@jcouv jcouv marked this pull request as ready for review February 18, 2022 18:50
@@ -253,7 +254,7 @@ public override bool Equals(Symbol? obj, TypeCompareKind compareKind)

return (object?)other != null &&
Equals(this.Locations.FirstOrDefault(), other.Locations.FirstOrDefault()) &&
this.ContainingAssembly.Equals(other.ContainingAssembly, compareKind);
Equals(this.ContainingSymbol, other.ContainingSymbol, compareKind);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContainingSymbol

Is there a reason to switch to ContainingSymbol? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we want the symbols not to be equal in the scenario with two compilations. But if we use ContainingAssembly (which is null) then they would be equal.
If we use ContainingSymbol (which is the global namespace) then they are not equal, which I think is correct.

We could only compare using ContainingSymbol when ContainingAssembly is null, if that seems better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a change in behavior then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous behavior for that scenario was a crash (null-ref). I don't think there's a change of behavior aside from that.
Am I missing something?

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 2)

@jcouv
Copy link
Member Author

jcouv commented Feb 19, 2022

@dotnet/roslyn-compiler for second review. Thanks

@jcouv jcouv changed the title Fix null-ref in AliasSymbol.Equals Fix null-ref in AliasSymbol.Equals Feb 19, 2022
@jcouv jcouv merged commit abd4cae into dotnet:main Feb 19, 2022
@jcouv jcouv deleted the alias-equals branch February 19, 2022 00:55
@ghost ghost added this to the Next milestone Feb 19, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AliasSymbol.Equals throw NullReferenceException when comparing two global symbols
4 participants